-
Notifications
You must be signed in to change notification settings - Fork 542
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add otelhttp.NewMiddleware #2964
Conversation
|
@alnr thanks for the contribution, please sign the CLA so these changes can be considered. |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #2964 +/- ##
=====================================
Coverage 79.3% 79.3%
=====================================
Files 165 165
Lines 10422 10426 +4
=====================================
+ Hits 8274 8278 +4
Misses 2012 2012
Partials 136 136
|
Hey. I'll do the paperwork early next year, sorry for the delay. Have a great new year! |
I also would like this change, will this be followed up? |
It looks like @alnr never signed the CLA. So we can't use his work there. |
I'll see if he answer today, if not I'll submit a followup tomorrow. |
Sorry, this got lost among the paperwork. CLA signed. |
|
||
// / NewMiddleware returns a tracing middleware from the given operation name and | ||
// options. | ||
func NewMiddleware(operation string, opts ...Option) *Middleware { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The middlewares I usually see are like the one for gorilla mux, in the form:
type MiddlewareFunc func(http.Handler) http.Handler
is this different in chi
? I think most http middleware library accepts this form.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
func(http.Handler) http.Handler
is most common, I agree. It's simple enough to write adapters for other uses.
I've pushed another commit, which gets rid of the otelhttp.Handler
type. That's not strictly backwards compatible. Is that OK?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've pushed another commit, which gets rid of the otelhttp.Handler type. That's not strictly backwards compatible. Is that OK?
I think it is fine as the type does not make any sense. However, in future I would rather propose to do such changes in a separate PR.
Rebased and fixed lint (I hope?) |
Gentle bump for review or merge :) |
@alnr Can you please update the branch (and resolve the conflicts)? |
…TTP routers and middleware chainers The key difference between the existing otelhttp.NewHandler and otelhttp.NewMiddleware is that the middleware takes the `next` handler as an argument after construction, wheres NewHandler works by wrapping one specific http.Handler at construction time.
done |
For better compatiblity with third-party HTTP routers and middleware chainers (e.g. github.com/urfave/negroni or github.com/go-chi/chi).
The key difference between the existing
otelhttp.Handler
and the newotelhttp.Middleware
is thatMiddleware
takes thenext
handler as an argument after construction, wheres the existingHandler
works by wrapping one specific http.Handler at construction time. The existing interfaces are unchanged.Is there interest in principle in merging this? If yes I'll deal with the CLA etc.